fix(orm): use id-only filter for update read-back #585
Conversation
* Prevents false `cannot-read-back` errors when update `returning` values don’t round-trip cleanly into an equality `where` filter * Fixes `update` read-back to derive the filter from model id fields instead of the full returned row * Addresses an issue that surfaces when multiple plugins are installed alongside `PolicyPlugin` (entity-mutation hooks can change mutation returning behavior and trigger policy read-back) * Adds a regression in the policy update CRUD suite covering JSON array field update with an additional entity-mutation plugin (runs on Postgres; skipped otherwise)
📝 WalkthroughWalkthroughThe update read-back filter now uses ID values extracted from the update result via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes false cannot-read-back errors in ORM update operations by using ID-only filters for read-back instead of the full updated row. This prevents issues when update returning values don't cleanly round-trip into equality filters, particularly when multiple plugins (including PolicyPlugin) are installed and affect mutation returning behavior.
Key changes:
- Modified update read-back to derive filter from model ID fields only
- Added regression test for JSON array field updates with mutation plugins on PostgreSQL
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/orm/src/client/crud/operations/update.ts | Changed runUpdate to use getIdValues() for extracting ID fields from update result for read-back filter, consistent with runUpsert and create operations |
| tests/e2e/orm/policy/crud/update.test.ts | Added PostgreSQL-specific regression test covering JSON array field updates with an entity-mutation plugin to ensure no false cannot-read-back errors occur |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/orm/policy/crud/update.test.ts (1)
1264-1349: Strong regression test for JSON array read-back behavior.The test validates that updating a JSON array field with a mutation plugin installed does not trigger a false
cannot-read-backerror. The PostgreSQL-gating is appropriate (JSON arrays are database-specific), and the test properly includes entity-mutation hooks to reproduce the scenario described in the PR objectives. The cleanup with$disconnect()in the finally block is correct.Optional refactor: Line 1341 includes
rootId: root.idin the update data, but foreign keys typically don't change during updates. Consider omitting it for clarity:♻️ Minor simplification
await expect( authed.jsonArrayField.update({ where: { id: created.id }, data: { data: updateData, - rootId: root.id, }, }), ).resolves.toMatchObject({ data: updateData });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/orm/src/client/crud/operations/update.tstests/e2e/orm/policy/crud/update.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/policy/crud/update.test.ts
🧠 Learnings (4)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/policy/crud/update.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/policy/crud/update.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
tests/e2e/orm/policy/crud/update.test.tspackages/orm/src/client/crud/operations/update.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/orm/src/client/crud/operations/update.ts
🧬 Code graph analysis (1)
packages/orm/src/client/crud/operations/update.ts (1)
packages/orm/src/client/query-utils.ts (1)
getIdValues(181-187)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: build-test (22.x, postgresql)
- GitHub Check: build-test (22.x, sqlite)
🔇 Additional comments (1)
packages/orm/src/client/crud/operations/update.ts (1)
46-46: Excellent fix for read-back filter robustness.Using
getIdValues()to derive the read-back filter from ID fields only (instead of the fullupdateResult) prevents falsecannot-read-backerrors when returned values don't round-trip cleanly into equality filters (e.g., JSON arrays, floating-point precision). This approach is consistent withrunUpdateManyAndReturn(line 114) andrunUpsert(line 163).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @tests/e2e/orm/policy/crud/update.test.ts:
- Line 2: The import getTestDbProvider is unused in this test; either remove
getTestDbProvider from the import list alongside createPolicyTestClient, or
actually use it to gate tests by provider (e.g., call getTestDbProvider() at the
top of the file and conditionally call describe.skip or return early when the
provider does not match the required DB). Update the import statement to only
include createPolicyTestClient if you choose removal, or add a small
provider-check block that references getTestDbProvider to skip or adjust tests
when needed.
In @tests/regression/test/issue-586.test.ts:
- Around line 1-87: Add a provider check so the test runs only for PostgreSQL:
import getTestDbProvider alongside createPolicyTestClient at the top and, inside
the it block for "does not throw cannot-read-back...", add an early return if
getTestDbProvider() !== 'postgresql' before calling createPolicyTestClient; keep
the rest of the test unchanged so sqlite CI iterations skip this
PostgreSQL-specific test.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/orm/src/client/crud/operations/update.tstests/e2e/orm/policy/crud/update.test.tstests/regression/test/issue-586.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/policy/crud/update.test.ts
🧠 Learnings (5)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/orm/src/client/crud/operations/update.tstests/e2e/orm/policy/crud/update.test.ts
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/orm/src/client/crud/operations/update.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/regression/test/issue-586.test.tstests/e2e/orm/policy/crud/update.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/regression/test/issue-586.test.tstests/e2e/orm/policy/crud/update.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
tests/e2e/orm/policy/crud/update.test.ts
🧬 Code graph analysis (2)
packages/orm/src/client/crud/operations/update.ts (1)
packages/orm/src/client/query-utils.ts (1)
getIdValues(181-187)
tests/regression/test/issue-586.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(258-269)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (22.x, postgresql)
- GitHub Check: build-test (22.x, sqlite)
🔇 Additional comments (1)
packages/orm/src/client/crud/operations/update.ts (1)
46-47: Excellent consistency improvement for read-back logic.This change aligns
runUpdatewith the read-back patterns already used inrunUpsert(line 170) andrunUpdateManyAndReturn(line 115), both of which callgetIdValuesto extract ID fields. Trimming to ID-only fields prevents falsecannot-read-backerrors when non-ID fields don't serialize cleanly for equality filters (e.g., JSON arrays, as demonstrated in the regression test).The conditional fallback to
args.wherecorrectly handles cases whereupdateResultis null or undefined. The implementation ofgetIdValuesextracts only the declared ID fields from the mutation result, ensuring the read-back query uses a minimal, stable filter.
|
fixes #586 |
2 participants
cannot-read-backerrors when updatereturningvalues don’t round-trip cleanly into an equalitywherefilterupdateread-back to derive the filter from model id fields instead of the full returned rowPolicyPlugin(entity-mutation hooks can change mutation returning behavior and trigger policy read-back)Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.